Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable the object-shorthand ESLint rule in web #8351

Merged
merged 1 commit into from
Apr 29, 2017
Merged

Enable the object-shorthand ESLint rule in web #8351

merged 1 commit into from
Apr 29, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

Since we're using Babel, it's now possible for us to use the more compact object notation available in ES6 (see this MDN article).

Please see http://eslint.org/docs/rules/object-shorthand; this is one of the rules mentioned in issue #7957.

Note: I initially created one big patch for this change, see master...Snuffleupagus:eslint-object-shorthand, but after looking at the diff I quickly realized that it was too big and I don't think anyone would want to review that. Hence I've decided to split it up into more manageable chunks instead.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/4e510e73d4202fe/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/4e510e73d4202fe/output.txt

Total script time: 1.57 mins

Published

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with the problem from the comment addressed.

web/hand_tool.js Outdated
onActiveChanged: function(isActive) {
this.eventBus.dispatch('handtoolchanged', {isActive: isActive});
}.bind(this)
onActiveChanged(isActive) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this change breaks the hand tool menu item in the secondary toolbar. Toggling the hand tool does work, but the menu no longer disappears after toggling and the text is not updated anymore. I'm also not sure if this is bound correctly without bind(this) in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh, this is a really stupid mistake on my part; thanks for spotting it!
I've updated the code to an arrow function, which is what it should have been from the start.

Please see http://eslint.org/docs/rules/object-shorthand.

For the most part, these changes are of the search-and-replace kind, and the previously enabled `no-undef` rule should complement the tests in helping ensure that no stupid errors crept into to the patch.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/43b85e35b180be0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/43b85e35b180be0/output.txt

Total script time: 1.57 mins

Published

@timvandermeij timvandermeij merged commit 5fb779d into mozilla:master Apr 29, 2017
@timvandermeij
Copy link
Contributor

Thank you!

@Snuffleupagus Snuffleupagus deleted the eslint_object-shorthand-web branch April 29, 2017 21:54
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…thand-web

Enable the `object-shorthand` ESLint rule in `web`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants